-
Notifications
You must be signed in to change notification settings - Fork 910
[PM-28086] Add testharness for Credential Manager and Autofill testing #6159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
New Issues (21)Checkmarx found the following issues in this Pull Request
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checkmarx One found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
9bfbdcd to
48f9872
Compare
|
Claude finished @SaintPatrck's task —— View job Code Review CompleteOverall Assessment: APPROVE with minor recommendations The Critical IssuesFinding 1: PR Description Missing Screenshots Finding 2: Incomplete Resolution of Human Reviewer Feedback
Finding 3: Author Dismissed Legitimate AndroidManifest Concern See inline comment on AndroidManifest.xml:20 for required fix. Architecture & Patterns✅ Strengths:
💭 Finding 4: Date Format Pattern in ViewModels See inline comment on CreatePasswordViewModel.kt:160 for recommendation. 💭 Finding 5: Navigation Pattern Inconsistency Current vs. Expected PatternCurrent (CredentialManagerListNavigation.kt:89-107): fun NavGraphBuilder.credentialManagerGraph(...) {
navigation<CredentialManagerGraphRoute>(...) {
credentialManagerListDestination(...) // ✅ Already extracted
// other destinations
}
}This is actually following the pattern correctly. The human reviewer's concern about "inline composableWithRootPushTransitions block" was already addressed - Status: Already resolved in latest commit. Code Quality🎨 Finding 6: Edge-to-Edge UI Padding Placement See inline comment on CreatePasswordScreen.kt:148 for fix that applies to all screen files. 🎨 Finding 7: String Resource Usage Inconsistency Current: Mixing
Not a blocker - current approach is reasonable for a test harness. Testing✅ Test Coverage: 94.4% patch coverage with 65 missing lines is excellent for initial implementation.
RecommendationAdd integration tests for Alternatively, document that manual testing coverage is sufficient for a test harness module. Security✅ Checkmarx Findings Review:
SummaryStrong implementation following Bitwarden Android patterns. Main concerns:
Recommendation: Address AndroidManifest and edge-to-edge padding issues before merge. Other findings are minor improvements that can be addressed in follow-up if needed. |
testharness/src/main/kotlin/com/bitwarden/testharness/data/manager/CredentialTestManagerImpl.kt
Show resolved
Hide resolved
...ss/src/main/kotlin/com/x8bit/bitwarden/testharness/data/manager/CredentialTestManagerImpl.kt
Outdated
Show resolved
Hide resolved
...ss/src/main/kotlin/com/x8bit/bitwarden/testharness/data/manager/CredentialTestManagerImpl.kt
Outdated
Show resolved
Hide resolved
...rc/test/kotlin/com/x8bit/bitwarden/testharness/data/manager/CredentialTestManagerImplTest.kt
Outdated
Show resolved
Hide resolved
...twarden/testharness/ui/platform/feature/credentialmanager/CredentialManagerListNavigation.kt
Show resolved
Hide resolved
Introduces a new standalone :testharness Android application module for end-to-end validation of Bitwarden's Credential Manager provider and Autofill framework implementations. Purpose: The test harness acts as a credential consumer client, allowing developers to trigger and verify all credential operations that Bitwarden supports as a provider without relying on external websites or applications. Supported Test Flows: - Password Creation: CreatePasswordRequest via Credential Manager - Password Retrieval: GetPasswordOption via Credential Manager - Passkey Creation: CreatePublicKeyCredentialRequest (FIDO2) - Passkey Authentication: GetPublicKeyCredentialOption (FIDO2) - Hybrid Operations: Combined password/passkey retrieval flows - Origin Parameter Testing: Privileged app simulation with custom origins - Autofill Framework: Placeholder for future autofill service testing Architecture: - MVVM + UDF: BaseViewModel with State/Action/Event patterns - Hilt DI: Full dependency injection throughout all layers - Jetpack Compose: Material 3 UI with BitwardenTheme integration - Navigation: Multi-screen architecture matching :app and :authenticator - Error Handling: Sealed Result classes, no exception-based handling Module Structure: - Activity layer: MainActivity with theme management - Navigation: RootNavScreen orchestrating all test flows - Data layer: CredentialTestManager wrapping AndroidX Credentials APIs - UI layer: Feature screens for each credential operation type - Test coverage: Comprehensive unit tests for all ViewModels and Screens Build Configuration: - Added :testharness to settings.gradle.kts - Integrated with detekt static analysis - Integrated with kover code coverage reporting - API 28+ requirement (AndroidX Credentials with Play Services) Requirements: - Android API 28+ - Bitwarden app installed as credential provider - Google Play Services (for API 28-33 compatibility) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
48f9872 to
634aa91
Compare
This commit addresses UI layout issues on testharness screens, particularly on devices with smaller displays where content could be pushed off-screen or overflow when the software keyboard is visible. The primary change is removing fixed heights and weights from the result `OutlinedTextField` components across multiple screens. These modifiers were making the text fields inflexible, preventing the layout from shrinking to accommodate the keyboard. By removing them, the text fields now dynamically resize based on their content, allowing the entire screen to scroll when necessary. Specific changes: - Removed fixed height and weight modifiers from result text fields in `CreatePasswordScreen`, `GetPasskeyScreen`, `GetPasswordScreen`, `CreatePasskeyScreen`, and `GetPasswordOrPasskeyScreen` to allow them to resize with their content. - Added vertical scrolling to `GetPasswordOrPasskeyScreen` to ensure all fields remain accessible when the keyboard is open. - Added bottom navigation bar padding to the `LandingScreen` to prevent the last button from being obscured by system navigation gestures.
This commit refactors the testing for `CredentialTestManagerImpl` by replacing the existing limited test file with a comprehensive new suite that covers all public methods and their various outcomes. The new test suite, `CredentialTestManagerTest`, provides detailed coverage for creating and retrieving both passwords and passkeys. It uses nested test classes for better organization and clarity. Key scenarios tested include successful operations (with and without an origin), user cancellations, API exceptions, and unexpected response types, ensuring the manager's logic is robust across different credential management flows. Specific changes: - Deleted the old, sparsely populated `CredentialTestManagerImplTest.kt`. - Created `CredentialTestManagerTest.kt` with comprehensive unit tests for: - `createPassword` - `getPassword` - `createPasskey` - `getPasskey` - `getPasswordOrPasskey` - Added tests for success, cancellation, and error states for each method. - Implemented mocking for various `androidx.credentials` classes and exceptions to simulate different API responses.
Exclude navigation boilerplate from coverage analysis as these files contain primarily routing logic that provides minimal value when tested in isolation. Files annotated: - AutofillPlaceholderNavigation.kt - CreatePasswordNavigation.kt - CreatePasskeyNavigation.kt - CredentialManagerListNavigation.kt - GetPasskeyNavigation.kt - GetPasswordNavigation.kt - GetPasswordOrPasskeyNavigation.kt - LandingNavigation.kt 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Create WebAuthnJsonBuilder utility class to separate WebAuthn JSON generation from credential management logic, as suggested in PR review. Changes: - Add WebAuthnJsonBuilder.kt with buildPasskeyCreationJson and buildPasskeyAuthenticationJson methods - Update CredentialTestManagerImpl to use WebAuthnJsonBuilder - Remove private JSON building methods from CredentialTestManagerImpl - Remove CHALLENGE_SEED_SIZE constant (moved to WebAuthnJsonBuilder) This improves separation of concerns and makes JSON building logic independently testable. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This commit addresses compiler warnings for unused exception variables in cancellation catch blocks. The exception variable `e` was declared but never used in `CreateCredentialCancellationException` and `GetCredentialCancellationException` handlers. The variable `e` has been replaced with `_` to signify that it is intentionally unused, resolving the warnings and improving code clarity.
Add the `@OmitFromCoverage` annotation to the `WebAuthnJsonBuilder` test harness utility. This object is used for generating test data and is not part of the production application code, so it should be excluded from code coverage metrics. Additionally, add warning comments to the `buildCreationRequest` and `buildAuthenticationRequest` methods, clarifying that the simplified challenge generation is for testing purposes only and not suitable for production use.
...ss/src/main/kotlin/com/x8bit/bitwarden/testharness/data/manager/CredentialTestManagerImpl.kt
Outdated
Show resolved
Hide resolved
...ss/src/main/kotlin/com/x8bit/bitwarden/testharness/data/manager/CredentialTestManagerImpl.kt
Outdated
Show resolved
Hide resolved
| navigation<AutofillGraphRoute>( | ||
| startDestination = AutofillPlaceholderRoute, | ||
| ) { | ||
| composable<AutofillPlaceholderRoute> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally we would have a AutofillPlaceholderNavigation file to put the route and destination.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I see what happened. Maybe we can still break inner destination out to a separate function.
Additionally, it should use the composableWithRootPushTransitions helper, right?
...in/com/x8bit/bitwarden/testharness/ui/platform/feature/autofill/AutofillPlaceholderScreen.kt
Outdated
Show resolved
Hide resolved
Adds navigationBarsPadding Spacer at the bottom of: - CreatePasswordScreen - GetPasswordScreen This ensures proper edge-to-edge display support and prevents content from being obscured by system navigation bars, maintaining consistency with other screens in the testharness. Addresses PR #6159 review feedback 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Remove Arrangement.spacedBy() patterns and replace with explicit Spacer components throughout testharness screens: - Remove spacedBy(16.dp) from main Columns - Add explicit Spacer(Modifier.height(16.dp)) between elements - Remove nested Columns used only for button spacing - Use explicit Spacer(Modifier.height(12.dp)) between buttons This provides better flexibility, clearer intent, and easier customization of individual spacing values. Improves readability by making all spacing explicit rather than implicit. Addresses PR #6159 review feedback 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
This commit refactors several screens within the `testharness` module to improve code consistency and align with standard UI patterns. The primary changes include: * Consolidating composable parameters into a single `state` object, simplifying the function signatures of `CreatePasskeyScreenContent` and its preview. * Replacing hardcoded horizontal padding (`padding(horizontal = 16.dp)`) with the `standardHorizontalMargin()` modifier for consistent layout spacing across various test harness screens. * Removing redundant vertical padding from list screens (`LandingScreen`, `CredentialManagerListScreen`) as spacing is handled by inner components. * Deleting an unused `Clock` provider from the `CredentialTestManagerModule`.
Inject a fixed `Clock` into the Credential-related view models (`CreatePasskeyViewModel`, `GetPasskeyViewModel`, `CreatePasswordViewModel`, `GetPasswordViewModel`, and `GetPasswordOrPasskeyViewModel`). This ensures that any time-dependent logic within these view models becomes deterministic, preventing potential flakiness in the corresponding tests. Previously, the view models relied on the system clock, which could lead to unpredictable behavior during test execution. This change makes the tests more reliable and repeatable.
Simplify the `compileOptions` block in `testharness/build.gradle.kts`. This change replaces the manual construction of the `JavaVersion` enum from a string with a more direct and readable method call. The previous implementation manually parsed the version string from `libs.versions.jvmTarget`, replaced the dot with an underscore, and prepended "VERSION_" to match the `JavaVersion` enum name. The new implementation achieves the same result by directly passing the version string to the `sourceCompatibility` and `targetCompatibility` methods, improving code clarity and maintainability.
|
Warning @SaintPatrck Uploading code coverage report failed. Please check the "Upload to codecov.io" step of Process Test Reports job for more details. |
...tlin/com/bitwarden/testharness/ui/platform/feature/createpassword/CreatePasswordViewModel.kt
Show resolved
Hide resolved
.../kotlin/com/bitwarden/testharness/ui/platform/feature/createpassword/CreatePasswordScreen.kt
Show resolved
Hide resolved
...twarden/testharness/ui/platform/feature/credentialmanager/CredentialManagerListNavigation.kt
Outdated
Show resolved
Hide resolved
Refactor the Credential Manager list navigation to use root push transitions and improve the screen's layout handling of system insets. - Switch `CredentialManagerListRoute` to use `composableWithRootPushTransitions` instead of the standard `composable` builder to ensure consistent transition animations. - Extract the screen instantiation into a private `credentialManagerListDestination` composable helper. - Add `navigationBarsPadding` and vertical spacing to the bottom of `CredentialManagerListScreen` to prevent content from being obscured by the system navigation bar. - Apply minor formatting improvements to the event handling logic in `CredentialManagerListScreen`.
Migrate the `testharness` module from the `com.x8bit.bitwarden.testharness` package to `com.bitwarden.testharness` to align with the project's standard package naming conventions. - Move all source and test files to the new `com.bitwarden.testharness` directory structure and update all import statements. - Update `namespace` and `applicationId` in `testharness/build.gradle.kts`. - Update package references in `testharness/proguard-rules.pro`. - Replace local `R.string.clear` usage with the shared `BitwardenString.clear` resource in `CreatePasskeyScreen` and `CreatePasswordScreen`. - Add a top spacer to the content list in `CredentialManagerListScreen` for improved layout spacing. - Improve code formatting by adding newlines between `when` expression branches in `CredentialTestManagerImpl`, `CredentialTestManagerTest`, and various ViewModels.
...twarden/testharness/ui/platform/feature/credentialmanager/CredentialManagerListNavigation.kt
Outdated
Show resolved
Hide resolved
.../kotlin/com/bitwarden/testharness/ui/platform/feature/createpassword/CreatePasswordScreen.kt
Outdated
Show resolved
Hide resolved
.../kotlin/com/bitwarden/testharness/ui/platform/feature/createpassword/CreatePasswordScreen.kt
Show resolved
Hide resolved
...tlin/com/bitwarden/testharness/ui/platform/feature/createpassword/CreatePasswordViewModel.kt
Show resolved
Hide resolved
.../kotlin/com/bitwarden/testharness/ui/platform/feature/createpassword/CreatePasswordScreen.kt
Show resolved
Hide resolved
Refactor navigation logic in the `testharness` module to use consistent transition patterns and improve UI scrolling behavior. Additionally, replace hardcoded strings with localized resources for accessibility content descriptions. - Refactor `AutofillPlaceholderNavigation` and `CredentialManagerListNavigation` to encapsulate screen definitions within `NavGraphBuilder` extension functions. These functions now directly utilize `composableWithRootPushTransitions`, ensuring consistent transition animations and cleaner graph definitions. - Update `CredentialManagerListScreen` to attach the `nestedScrollConnection` from the `scrollBehavior` to the `BitwardenScaffold`. This enables proper nested scrolling interactions with the pinned top app bar. - Replace the hardcoded "Back" content description with `stringResource(BitwardenString.back)` across all feature screens (`CreatePasswordScreen`, `CreatePasskeyScreen`, `GetPasswordScreen`, `GetPasskeyScreen`, `GetPasswordOrPasskeyScreen`, and `CredentialManagerListScreen`) to ensure consistency and proper localization support. - Delete the unused helper script `.claude/skills/reviewing-changes/scripts/detect-threads.sh`.
|
|
||
| <activity | ||
| android:name=".MainActivity" | ||
| android:configChanges="uiMode" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Missing critical configuration changes
The configChanges attribute must include orientation|screenSize in addition to uiMode. Without these:
- Device rotation will destroy and recreate the activity
- Multi-window/foldable device changes will destroy and recreate the activity
SavedStateHandleusage alone doesn't prevent the performance cost and visual disruption
Required fix
android:configChanges="orientation|screenSize|uiMode"Why this matters:
orientation: Handles device rotation without recreationscreenSize: Handles multi-window mode and foldable device configuration changesuiMode: Handles dark/light theme changes (already present)
The main :app module follows this same pattern. See app/src/main/AndroidManifest.xml for reference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional. We are only concerned with uiMode changes.
| ) | ||
|
|
||
| Spacer(modifier = Modifier.height(16.dp)) | ||
| Spacer(modifier = Modifier.navigationBarsPadding()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎨 NavigationBarsPadding inside scrolling content breaks edge-to-edge
The navigationBarsPadding() modifier should not be inside the scrolling Column as a Spacer. This causes the padding to scroll with content, breaking edge-to-edge display.
Standard pattern from :app module
Apply padding to the Column modifier itself:
Column(
modifier = Modifier
.fillMaxSize()
.imePadding()
.navigationBarsPadding() // Move here
.verticalScroll(rememberScrollState()),
) {
// ... content ...
Spacer(modifier = Modifier.height(16.dp))
// Remove: Spacer(modifier = Modifier.navigationBarsPadding())
}Applies to these screens:
- CreatePasswordScreen.kt:148
- CreatePasskeyScreen.kt (similar pattern)
- GetPasswordScreen.kt (similar pattern)
- GetPasskeyScreen.kt (similar pattern)
- GetPasswordOrPasskeyScreen.kt (similar pattern)
- CredentialManagerListScreen.kt (similar pattern)
Reference: See vault item screens in :app module for correct edge-to-edge implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@claude We want the padding to scroll with the content
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Claude finished @david-livefront's task —— View job
Clarification Acknowledged
Thank you for the clarification, David. I understand now that having the navigationBarsPadding() scroll with the content is the intended behavior for this test harness.
I'm withdrawing my previous comment about this being an edge-to-edge display issue. The current implementation with Spacer(modifier = Modifier.navigationBarsPadding()) inside the scrolling Column is correct for the desired UX in this context.
This pattern applies across the testharness screens (CreatePasswordScreen, CreatePasskeyScreen, GetPasswordScreen, etc.) and is intentional.
| } | ||
|
|
||
| private fun timestamp(): String { | ||
| val formatter = DateTimeFormatter.ofPattern("HH:mm:ss") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 Hardcoded date format pattern
The timestamp format "HH:mm:ss" is hardcoded and not locale-aware. For a test harness, this is likely intentional for consistent log parsing, but should be documented.
Add documentation
private fun timestamp(): String {
// Fixed format for consistent test output parsing across locales
val formatter = DateTimeFormatter.ofPattern("HH:mm:ss")
return clock.instant()
.atZone(ZoneId.systemDefault())
.format(formatter)
.let { "[$it]" }
}If locale-aware formatting is desired instead:
private fun timestamp(): String {
val formatter = DateTimeFormatter.ofLocalizedTime(FormatStyle.MEDIUM)
return clock.instant()
.atZone(ZoneId.systemDefault())
.format(formatter)
.let { "[$it]" }
}Applies to: All ViewModel files with timestamp() functions (CreatePasskeyViewModel, GetPasswordViewModel, GetPasskeyViewModel, GetPasswordOrPasskeyViewModel)
aj-rosado
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work @SaintPatrck. LGTM 👏
|
Thanks @aj-rosado and @david-livefront. |


🎟️ Tracking
PM-28086
📔 Objective
This PR introduces a new
:testharnessmodule - a standalone Android application for validating Bitwarden's Credential Manager provider implementation.Purpose:
The test harness allows developers to exercise and verify all Credential Manager API operations that the Bitwarden app supports as a credential provider:
CreatePasswordRequestandGetPasswordOptionCreatePublicKeyCredentialRequestandGetPublicKeyCredentialOptionArchitecture:
The module follows the same patterns as
:appand:authenticator:BaseViewModelIntegration:
The test harness acts as a credential consumer client, triggering the system credential picker and allowing selection of Bitwarden as the provider. This enables end-to-end validation of credential flows without relying on external websites or apps.
Build Configuration:
Added
:testharnessto detekt (static analysis) and kover (code coverage) configurations in rootbuild.gradle.kts.Requirements: API 28+ (uses AndroidX Credentials library with Play Services compatibility)
📸 Screenshots
Coming soon!
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes